Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

async-tungstenite: Make TLS features optional #222

Merged
merged 2 commits into from
Jul 5, 2024
Merged

Conversation

felinira
Copy link
Collaborator

The included TLS features are only required for wss connections to the mailbox server which not required by the protocol and only really useful for wasm builds.

Closes #216

Copy link

codecov bot commented Apr 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 39.24%. Comparing base (6082d8b) to head (7366377).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #222   +/-   ##
=======================================
  Coverage   39.24%   39.24%           
=======================================
  Files          18       18           
  Lines        3088     3088           
=======================================
  Hits         1212     1212           
  Misses       1876     1876           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@piegamesde piegamesde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally I'd like to have the default rendezvous server listen to wss by default, but until that happens (it honestly won't anytime soon anyways) this is probably the right thing to do.

@felinira
Copy link
Collaborator Author

Either way we need a way to select the backend here. I would personally choose native TLS for example, to get rid of rustls and ring. I can make it a default feature instead.

wss support doesn't really give us much though, does it? What advantage do we have from connecting to a server via wss? From what I gathered the reason for wss support is mostly due to wasm requirements?

The included TLS features are only required for wss connections to the mailbox
server which not required by the protocol and only really useful for wasm
builds.

Closes #216
@felinira felinira force-pushed the fina/tls-features branch from 41ceb52 to 7366377 Compare April 22, 2024 19:09
@meejah
Copy link
Member

meejah commented Apr 23, 2024

wss support doesn't really give us much though, does it?

Not really, it's already end-to-end encrypted in the protocol itself (and authenticated).

I believe at least part of the point of the default server being TCP-only is to prove this point (although I've not seen this written down anywhere by Brian).

@felinira
Copy link
Collaborator Author

I'll leave it out of default features for now. Cargo doesn't support target-specific default features so we can't auto-enable it on wasm either (wasm being the only place where it makes sense to both enable it by default, and native tls doesn't exist)

@meejah
Copy link
Member

meejah commented Apr 24, 2024

Yes, typically WASM will require wss://

@felinira felinira merged commit 9e53e6e into master Jul 5, 2024
14 checks passed
@felinira felinira deleted the fina/tls-features branch July 5, 2024 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose the TLS implementation features of async-tungstenite
3 participants